Inline Redis collection TTL metadata#997
Conversation
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughRedisのTTLをコレクションメタデータとHLL値へインライン化し、型別のTTL読み書き、トランザクション、レガシーデータ移行、デルタコンパクション、期限切れ再作成、バックアップ復元を更新しました。 ChangesTTLインライン化と互換性
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements inline TTL for Redis collections (list, hash, set, zset, stream) and HLL payloads by embedding expiration timestamps directly in their metadata anchors, supported by a background migrator and an option to disable the legacy secondary index fallback. The code review identified several critical issues: first, applyPositiveExpire does not update collection metadata with the new TTL, leading to desynchronization; second, the background migrator batches too many keys in a single transaction, risking frequent write conflicts; and third, both hasExpired and decodeHLLSetValueAt bypass the disableLegacyTTLReadFallback gate by unconditionally calling legacyIndexTTLAt. Additionally, the migrator should be made more robust to handle legacy TTL updates on already-migrated keys.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if err := c.dispatchCompaction(ctx, readTS, elems); err != nil { | ||
| c.logger.WarnContext(ctx, "ttl inline migrator: batch dispatch failed", | ||
| "type", h.typeName, "error", err) | ||
| return err | ||
| } |
There was a problem hiding this comment.
migrateTTLInlineHandler accumulates migration elements for up to 512 different user keys (ttlInlineMigrationTickScanLimit) and dispatches them all in a single Raft transaction via c.dispatchCompaction. In a production environment, a single transaction containing up to 1024 elements (migration writes + TTL index syncs) across hundreds of distinct keys is highly susceptible to write conflicts. If a concurrent client write touches even one of these keys during the transaction, the entire batch of 512 keys will fail with ErrWriteConflict and abort. This leads to wasted CPU/network resources, high transaction retry overhead, and can severely slow down or stall the background migration. Recommendation: Batch the migration dispatches into much smaller groups (e.g., 16 or 32 keys per transaction), or dispatch them individually/in very small batches so that a write conflict on a single hot key does not roll back the migration progress of the entire scanned window.
| if err == nil && !found { | ||
| ttl, err = r.legacyIndexTTLAt(ctx, userKey, readTS) | ||
| } |
There was a problem hiding this comment.
In hasExpired, when nonStringOnly is true and collectionTTLAt misses (found == false), legacyIndexTTLAt is called unconditionally. This completely bypasses the r.disableLegacyTTLReadFallback gate. Even after the operator disables the legacy fallback to optimize hot-path reads, any non-string check (such as EXISTS on a non-existent key or a key without a TTL) will still perform an expensive, redundant point lookup on the !redis|ttl|<key> index. Recommendation: Wrap the legacyIndexTTLAt call in a check for !r.disableLegacyTTLReadFallback.
| if err == nil && !found { | |
| ttl, err = r.legacyIndexTTLAt(ctx, userKey, readTS) | |
| } | |
| if err == nil && !found && !r.disableLegacyTTLReadFallback { | |
| ttl, err = r.legacyIndexTTLAt(ctx, userKey, readTS) | |
| } |
| if ttlMs == 0 { | ||
| ttlMs, err = legacyTTLMillisAt(ctx, c.st, userKey, readTS) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
In simpleTTLInlineMigrationHandler, the migrator only queries the legacy TTL index via legacyTTLMillisAt if ttlMs == 0 (i.e., the metadata key is in the legacy format or has no inline TTL). If a collection key has already been migrated to the inline format (so ttlMs > 0), but a subsequent EXPIRE command executed inside a MULTI/EXEC transaction or a Lua script updates the legacy TTL index without updating the metadata key, the migrator will completely skip reading the legacy TTL index. As a result, the metadata key will remain stuck with the stale TTL, and ttlAt (which prioritizes inline metadata) will return the stale TTL. While the primary fix should be ensuring that transactions and Lua scripts update the metadata key directly, the migrator could also be made more robust by always syncing the metadata TTL if the legacy TTL index has a different, newer value during the migration window.
| if !embedded { | ||
| ttl, err = r.legacyIndexTTLAt(ctx, key, readTS) | ||
| if err != nil { | ||
| return redisSetValue{}, err | ||
| } | ||
| } |
There was a problem hiding this comment.
In decodeHLLSetValueAt, if the HLL payload is in the legacy format (embedded == false), legacyIndexTTLAt is called unconditionally. This ignores the r.disableLegacyTTLReadFallback gate, meaning we will still perform a secondary index lookup for legacy HLL keys even after the fallback has been disabled. Recommendation: Respect r.disableLegacyTTLReadFallback before calling legacyIndexTTLAt.
| if !embedded { | |
| ttl, err = r.legacyIndexTTLAt(ctx, key, readTS) | |
| if err != nil { | |
| return redisSetValue{}, err | |
| } | |
| } | |
| if !embedded && !r.disableLegacyTTLReadFallback { | |
| ttl, err = r.legacyIndexTTLAt(ctx, key, readTS) | |
| if err != nil { | |
| return redisSetValue{}, err | |
| } | |
| } |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50d34a5e09
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Handled the latest TTL-inline review blockers.
Validation:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 607ee9c82d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Handled the latest TTL-inline review blockers.
Validation:
Note: go test ./adapter -count=1 -timeout=300s reached the package-level timeout after the focused coverage and lint above had passed. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b26fd2a95
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Fixed the latest HLL TTL precedence finding. Changes:
Caller audit:
Validation:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd45d8ccfb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Handled the latest TTL-inline review blockers.
Validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1b3251649
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if legacyTTL != 0 && legacyTTL != ttlMs { | ||
| ttlMs = legacyTTL |
There was a problem hiding this comment.
When this handler scans an already-current hash/set/zset meta value, ttlMs came from the inline metadata and is authoritative, but this fallback unconditionally re-imports any stale !redis|ttl| row. For a current 16-byte meta with ExpireAt=0 plus an old scan-index TTL, the migrator rewrites the collection with that stale TTL and the key can later expire even though the inline anchor says it is persistent. Only consult the legacy TTL when pair.Value is the legacy 8-byte meta format.
Useful? React with 👍 / 👎.
| if legacyTTL != 0 && legacyTTL != meta.ExpireAt { | ||
| meta.ExpireAt = legacyTTL |
There was a problem hiding this comment.
For a current-format list meta row, meta.ExpireAt is already the authoritative TTL, including 0 for no TTL; this unconditional legacy fallback will copy a stale !redis|ttl| value back into the inline meta. A list that was made persistent but still has an old scan-index row will be rewritten as expiring by the migrator. Gate this fallback on the legacy 24-byte list meta format; the stream migrator below repeats the same pattern and needs the same guard for 24-byte stream meta.
Useful? React with 👍 / 👎.
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Handled the latest inline TTL metadata blockers on Changes:
Caller audit:
Validation:
@codex review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfea1eb590
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| } | ||
| value, err := r.loadSetAt(ctx, hllKind, key, readTS) | ||
| value, _, err := r.loadHLLPayloadAt(ctx, key, readTS) |
There was a problem hiding this comment.
For an HLL whose inline TTL has expired but whose physical !redis|hll| row has not been swept yet, the preceding TTL-filtered keyTypeAt returns redisTypeNone, but this unfiltered load still decodes the anchor and unions its members. In that window PFCOUNT key returns the stale cardinality instead of treating the key as absent, so this should use the TTL-filtered HLL load/existence path.
Useful? React with 👍 / 👎.
| if ttlMs == 0 { | ||
| ttlMs, err = legacyTTLMillisAt(ctx, c.st, userKey, readTS) |
There was a problem hiding this comment.
This fallback runs whenever the decoded ExpireAt is zero, including current inline metadata that explicitly means “no TTL”. If such a key has a stale !redis|ttl| scan-index entry, the migrator copies that stale timestamp back into the authoritative inline meta (the list/stream handlers use the same pattern), so persistent collections can become expiring after a compaction pass; only consult legacyTTLMillisAt for legacy-width metadata.
Useful? React with 👍 / 👎.
| case redisTypeNone, redisTypeString, redisTypeList, redisTypeStream: | ||
| return nil, nil |
There was a problem hiding this comment.
When the TTL-index migrator encounters a legacy !redis|stream| blob with no new !stream|meta| anchor, this branch skips it entirely, unlike the hash/set/zset legacy blob cases below. After WithRedisLegacyTTLReadFallback(false) is enabled following migration, those legacy streams no longer consult !redis|ttl|, so an expired legacy stream is treated as live until some later write happens to clean it up.
Useful? React with 👍 / 👎.
|
Handled the latest TTL-inline review blockers on Changes:
Caller audit:
Validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62e29c2c49
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Handled the latest TTL-inline restore and recreate blockers on Changes:
Validation:
Note: Current unresolved non-outdated review threads: 0. @codex review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
adapter/redis_lua_context.go (1)
3178-3186: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick wincommit の ctx を下流の store 読み取りまで伝播してください
nonStringTTLElemsとdirtyPositiveTTLOnLogicallyAbsentStartがcontext.Background()でcollectionExpireElems/keyTypeAtを呼んでいるため、redisDispatchTimeoutの期限が効きません。commit()で作ったctxを引数で渡し、commitPlanForKey内のkeyTypeAtも合わせて置き換えてください。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_lua_context.go` around lines 3178 - 3186, commit() で生成した ctx が下流のストア読み取りまで伝播するよう修正してください。nonStringTTLElems と dirtyPositiveTTLOnLogicallyAbsentStart に ctx を引数として追加し、内部の collectionExpireElems および keyTypeAt 呼び出しで context.Background() を使わず渡された ctx を使用してください。commitPlanForKey 内の keyTypeAt 呼び出しも同じ ctx に置き換え、各呼び出し元を更新して redisDispatchTimeout が適用されるようにしてください。
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@adapter/redis_collection_ttl.go`:
- Around line 359-377: In simpleMetaExpireScanErr, invoke
r.triggerUrgentCompaction(typeName, key) in the ErrDeltaScanTruncated branch
even when exists is true, before returning the metadata update, so delta folding
is triggered consistently with the other collection types.
---
Nitpick comments:
In `@adapter/redis_lua_context.go`:
- Around line 3178-3186: commit() で生成した ctx
が下流のストア読み取りまで伝播するよう修正してください。nonStringTTLElems と
dirtyPositiveTTLOnLogicallyAbsentStart に ctx を引数として追加し、内部の collectionExpireElems
および keyTypeAt 呼び出しで context.Background() を使わず渡された ctx を使用してください。commitPlanForKey
内の keyTypeAt 呼び出しも同じ ctx に置き換え、各呼び出し元を更新して redisDispatchTimeout が適用されるようにしてください。
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ab049491-393b-4d1e-8af2-31bf37780eb7
📒 Files selected for processing (41)
adapter/redis.goadapter/redis_collection_ttl.goadapter/redis_collection_ttl_test.goadapter/redis_compat_commands_stream_test.goadapter/redis_compat_helpers.goadapter/redis_compat_types.goadapter/redis_delta_compactor.goadapter/redis_delta_compactor_test.goadapter/redis_expire_cmds.goadapter/redis_hash_cmds.goadapter/redis_hll_ttl_test.goadapter/redis_lua_context.goadapter/redis_proxy_leader.goadapter/redis_retry_test.goadapter/redis_set_cmds.goadapter/redis_storage_codec_test.goadapter/redis_stream_cmds.goadapter/redis_ttl_compat_test.goadapter/redis_ttl_inline_migrator.goadapter/redis_txn.goadapter/redis_txn_test.goadapter/redis_zset_cmds.godocs/design/2026_04_17_implemented_ttl_inline_value.mdinternal/backup/encode_redis.gointernal/backup/encode_redis_coll.gointernal/backup/encode_redis_coll_test.gointernal/backup/encode_redis_test.gointernal/backup/redis_hash.gointernal/backup/redis_hash_test.gointernal/backup/redis_list.gointernal/backup/redis_meta.gointernal/backup/redis_set.gointernal/backup/redis_stream.gointernal/backup/redis_string.gointernal/backup/redis_string_test.gointernal/backup/redis_zset.gostore/hash_helpers.gostore/list_helpers.gostore/set_helpers.gostore/stream_helpers.gostore/zset_helpers.go
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aceb441fd7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| keyBytes := []byte(key) | ||
| elems := []*kv.Elem[kv.OP]{} | ||
| if preserveExisting { | ||
| metaElems, ok, err := c.server.collectionExpireElems(context.Background(), keyBytes, c.startTS, typ, ttlMillis(ttl)) |
There was a problem hiding this comment.
When a Lua script mutates a ZSET through the delta path and also changes its TTL while exactly store.MaxDeltaScanLimit existing ZSetMetaDeltaKey rows are present, zsetDeltaMetaElems can compact those old deltas together with the script's current lenDelta into a new base meta earlier in the same transaction. This later collectionExpireElems call rebuilds the meta again from c.startTS without the current lenDelta and is appended after the ZSET elems, so it overwrites the compacted meta with a stale cardinality while the member rows have already changed; ZCARD/empty detection then observe the wrong length until a later repair.
Useful? React with 👍 / 👎.
| r.triggerUrgentCompaction(typeName, key) | ||
| return nil, true, nil |
There was a problem hiding this comment.
When a hash/set/zset exists only as delta rows and has more than MaxDeltaScanLimit deltas, this branch reports success to EXPIRE while emitting no inline metadata update, so only the !redis|ttl| scan row is written. The urgent compactor that runs afterward folds those deltas into a current 16-byte meta with ExpireAt=0, and the TTL migrator then skips the scan row because a base meta exists, leaving the key permanently unexpired (and even the fallback path is shadowed by the zero inline TTL).
Useful? React with 👍 / 👎.
|
Addressed the latest inline TTL review feedback. Changes:
Verification:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66d868cdee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if exists { | ||
| return listMetaTTLUpdateElem(key, meta, expireAtMs) | ||
| } | ||
| return nil, true, nil |
There was a problem hiding this comment.
For a list that only has ListMetaDeltaKey rows and more than MaxDeltaScanLimit deltas, this branch returns ok=true with no metadata update, so standalone EXPIRE and MULTI/EXEC will still write only !redis|ttl| and report success. The urgent compactor then materializes a 32-byte list meta with ExpireAt=0, and the inline TTL migrator treats that as authoritative and can delete the scan-index TTL, so the list silently loses the expiry after compaction/migration.
Useful? React with 👍 / 👎.
|
Addressed latest-head TTL review findings:
Caller audit:
Validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30c54f9f61
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if embedded || newFormat { | ||
| return ttl, true, nil | ||
| } | ||
| return nil, false, nil |
There was a problem hiding this comment.
When an HLL anchor is still in the legacy payload format, this returns found=false, so ttlAt continues into collectionTTLAt before consulting the legacy !redis|ttl| index. In the same stale-metadata scenario covered for new HLL envelopes (a leftover hash/list/set/zset meta row for the same user key), a pre-migration HLL can be reported expired or assigned the collection's TTL even though decodeHLLSetValueAt would otherwise use the HLL's own legacy TTL/no-TTL state. Treat the presence of the HLL anchor as authoritative here, falling back to the legacy TTL index for old HLL payloads instead of letting collection metadata win.
Useful? React with 👍 / 👎.
|
Fixed the current Changes:
Validation:
Current head: @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 248ae1045d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| elems = append(elems, &kv.Elem[kv.OP]{ | ||
| Op: kv.Put, | ||
| Key: store.StreamEntryKey(userKey, parsed.ms, parsed.seq), | ||
| Value: entryValue, | ||
| }) |
There was a problem hiding this comment.
For a pre-wide-column stream that still has a !redis|ttl| row, the TTL-inline migrator now materializes every legacy stream entry into !stream|entry| rows. That contradicts the stream migration contract in the read/write path, where legacy stream blobs are ignored by loadStreamAt and deleted on the next write, so a background TTL migration can make previously discarded legacy entries visible again to XLEN/XRANGE/XREAD.
Useful? React with 👍 / 👎.
|
Addressed the current-head TTL-inline review blockers. Changes:
Caller audit:
Verification:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 751f0547c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
adapter/redis_lua_context.go (1)
3393-3407: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winフルコミット時に
ListMeta.Tailを設定してください。要素はシーケンス
0..len-1に保存されますが、Tailがゼロのため、永続化された範囲とメタデータが不整合になります。修正案
keyBytes := []byte(key) elems := make([]*kv.Elem[kv.OP], 0, len(st.values)+listFullCommitExtraElems) for seq, value := range st.values { elems = append(elems, &kv.Elem[kv.OP]{ Op: kv.Put, Key: listItemKey(keyBytes, int64(seq)), Value: []byte(value), }) } - meta, err := store.MarshalListMeta(store.ListMeta{Len: int64(len(st.values)), ExpireAt: ttlMillis(ttl)}) + listLen := int64(len(st.values)) + meta, err := store.MarshalListMeta(store.ListMeta{ + Head: 0, + Tail: listLen, + Len: listLen, + ExpireAt: ttlMillis(ttl), + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_lua_context.go` around lines 3393 - 3407, フルコミット時に生成する ListMeta の Tail が未設定です。要素をシーケンス 0..len-1 で保存している処理と対応するよう、MarshalListMeta に渡す store.ListMeta で Tail を st.values の末尾シーケンス（空の場合も適切に扱う）として設定してください。対象はこの elems 構築処理と store.MarshalListMeta 呼び出しです。
🧹 Nitpick comments (1)
adapter/redis_zset_cmds.go (1)
617-617: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win既存定数を使って
//nolint:mndを削除してください。
zsetElemsPerAddedが同じ最大3要素を表しているため、リテラルと抑制コメントは不要です。修正案
- elems := make([]*kv.Elem[kv.OP], 0, len(base.cleanupElems)+len(base.migrationElems)+len(pairs)*3+setWideColOverhead) //nolint:mnd // 3 ops per pair + elems := make([]*kv.Elem[kv.OP], 0, len(base.cleanupElems)+len(base.migrationElems)+len(pairs)*zsetElemsPerAdded+setWideColOverhead)- elems := make([]*kv.Elem[kv.OP], 0, len(base.cleanupElems)+len(base.migrationElems)+3) //nolint:mnd // del old score + put member + put score index + elems := make([]*kv.Elem[kv.OP], 0, len(base.cleanupElems)+len(base.migrationElems)+zsetElemsPerAdded)As per coding guidelines, 「Avoid using
//nolintdirectives; refactor instead in Go code」.Also applies to: 746-746
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_zset_cmds.go` at line 617, Replace the literal multiplier 3 and its //nolint:mnd suppression in the capacity calculations involving cleanupElems, migrationElems, and pairs with the existing zsetElemsPerAdded constant; apply the same change to the corresponding calculation near the second referenced location.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@adapter/redis_lua_context.go`:
- Around line 3393-3407: フルコミット時に生成する ListMeta の Tail が未設定です。要素をシーケンス 0..len-1
で保存している処理と対応するよう、MarshalListMeta に渡す store.ListMeta で Tail を st.values
の末尾シーケンス（空の場合も適切に扱う）として設定してください。対象はこの elems 構築処理と store.MarshalListMeta 呼び出しです。
---
Nitpick comments:
In `@adapter/redis_zset_cmds.go`:
- Line 617: Replace the literal multiplier 3 and its //nolint:mnd suppression in
the capacity calculations involving cleanupElems, migrationElems, and pairs with
the existing zsetElemsPerAdded constant; apply the same change to the
corresponding calculation near the second referenced location.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3d0e73b8-c757-4d6f-9201-19a09c95233b
📒 Files selected for processing (11)
adapter/redis_collection_ttl.goadapter/redis_collection_ttl_test.goadapter/redis_compat_helpers.goadapter/redis_compat_types.goadapter/redis_hash_cmds.goadapter/redis_hll_ttl_test.goadapter/redis_lists.goadapter/redis_lua_context.goadapter/redis_set_cmds.goadapter/redis_ttl_inline_migrator.goadapter/redis_zset_cmds.go
💤 Files with no reviewable changes (1)
- adapter/redis_compat_helpers.go
🚧 Files skipped from review as they are similar to previous changes (5)
- adapter/redis_hll_ttl_test.go
- adapter/redis_compat_types.go
- adapter/redis_set_cmds.go
- adapter/redis_collection_ttl.go
- adapter/redis_ttl_inline_migrator.go
|
Addressed the latest stream TTL cleanup gap on the current head. Changes:
Caller audit:
Validation:
Current CI for @codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Resolved the remaining addressed review threads on the current head after rechecking the implementation and focused tests. Verification:
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Design doc
Validation
Author: bootjp
Summary by CodeRabbit